Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MultiThread debug element memory leak #1564

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Oct 3, 2024

I saw a memory leak which may (still under tests) be the source of my recent failing tests on LG TVs (the application restarted after one or two hours of playback).

It only appears in a combination of experimental features, so not dramatic either:

  • multithread is enabled
  • the RxPlayer's debug element is shown on screen

I noticed that a debug-related Promise's reject callback was correctly linked to the Init's CancellationSignal but was never unbound from it.

As such, that CancellationSignal, which is alive as long as the content is playing, would keep a reference to that reject function, and this seems to extend to other objects linked to that Promise. I was for example under the impression that even that Promise's resolved value was retained by looking at Chrome's inspector's "memory tab".

The solution is simply to unbind reject from the CancellationSignal when either "rejecting" or "resolving" the Promise.


I also profited from this fix to transform the resolvers object from a simple JS Object to a Map as it seems much more appropriate: we most frequently insert and remove values from it, and we never need to serialize that object.

Also that Object's keys were number and to my understanding JS Object keys are always stringified (or at least you cannot have e.g. [1] and ["1"] point to different values) so that didn't seem natural to me, plus the fact that I initially feared that it was an array.

@peaBerberian peaBerberian added bug This is an RxPlayer issue (unexpected result when comparing to the API) Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first. labels Oct 3, 2024
@peaBerberian peaBerberian added this to the 4.2.0 milestone Oct 3, 2024
I saw a memory leak which may (still under tests) be the source of my
recent failing tests on LG TVs (the application restarted after one or
two hours of playback).

It only appears in a combination of experimental features, so not
dramatic either:
  - multithread is enabled
  - the RxPlayer's debug element is shown on screen

I noticed that the create Promise's `reject` callback was correctly
linked to the `Init`'s CancellationSignal but was never unbound from it.

As such, that `CancellationSignal`, which is alive as long as the
content is playing, would keep a reference to that reject's function,
and this seems to extend to other objects linked to that Promise. I was
for example under the impression that even that Promise's resolved value
was retained by looking at Chrome's inspector.

The solution is simply to unbound `reject` from the CancellationSignal
when either "rejecting" or "resolving" the Promise.

---

I also profited from this fix to transform the `resolvers` object from a
simple JS Object to a `Map` as it seems much more appropriate: we
very frequently insert and remove values from it, and we never need to
serialize that object.

Also that Object's keys were number and to my understanding JS Object
keys are always stringified (or at least you cannot have e.g. `[1]`
and `["1"]` point to different values) so that didn't seem natural to
me, plus the fact that I initially feared that it was an array.
@peaBerberian peaBerberian force-pushed the fix/debug-elt-memory-leak branch from cf3e5e4 to a6fada4 Compare October 5, 2024 09:28
@peaBerberian peaBerberian merged commit 789a75a into dev Oct 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is an RxPlayer issue (unexpected result when comparing to the API) Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants